Skip to content

Add optional rapid sync timestamp field to NetworkGraph #1500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 31, 2022
Merged

Add optional rapid sync timestamp field to NetworkGraph #1500

merged 1 commit into from
May 31, 2022

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented May 27, 2022

This field will be necessary for rapid gossip sync to not burden users with a one-off variable requiring separate storage.

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2022 •
edited
Loading

Codecov Report

Merging #1500 (fd75747) into main (aac3907) will increase coverage by 1.22%.
The diff coverage is 93.33%.

❗ Current head fd75747 differs from pull request most recent head c3bbfe5. Consider uploading reports for the commit c3bbfe5 to get more accurate results

@@            Coverage Diff             @@
##             main    #1500      +/-   ##
==========================================
+ Coverage   90.95%   92.17%   +1.22%     
==========================================
  Files          80       80              
  Lines       42724    52517    +9793     
  Branches    42724    52517    +9793     
==========================================
+ Hits        38858    48407    +9549     
- Misses       3866     4110     +244
Impacted Files Coverage Δ
lightning/src/routing/network_graph.rs 90.99% <93.33%> (+0.01%) ⬆️
lightning/src/ln/priv_short_conf_tests.rs 97.09% <0.00%> (-0.87%) ⬇️
lightning/src/util/scid_utils.rs 98.75% <0.00%> (-0.45%) ⬇️
lightning/src/ln/payment_tests.rs 99.18% <0.00%> (-0.05%) ⬇️
lightning/src/ln/reorg_tests.rs 100.00% <0.00%> (ø)
lightning/src/ln/monitor_tests.rs 100.00% <0.00%> (ø)
lightning/src/chain/onchaintx.rs 94.90% <0.00%> (+0.10%) ⬆️
lightning/src/util/config.rs 44.21% <0.00%> (+0.21%) ⬆️
lightning/src/chain/chainmonitor.rs 98.13% <0.00%> (+0.21%) ⬆️
lightning-rapid-gossip-sync/src/lib.rs 90.62% <0.00%> (+0.30%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aac3907...c3bbfe5. Read the comment docs.

TheBlueMatt
TheBlueMatt reviewed May 27, 2022
View reviewed changes
lightning/src/routing/network_graph.rs Outdated
@@ -127,6 +127,7 @@ pub struct NetworkGraph {
// Lock order: channels -> nodes
channels: RwLock<BTreeMap<u64, ChannelInfo>>,
nodes: RwLock<BTreeMap<NodeId, NodeInfo>>,
last_rapid_gossip_sync_timestamp: Option<u32>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be pub, documented, and updated automatically by the lightning-graph-sync crate stuff, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes to 1) and 2), but 3) should be done in a separate PR, preferably the one with the background processor pruning.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe lets just do this in that PR? The docs need to talk about when its updated and how to use it, and that would imply changes to the lightning-rapid-sync crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol I thought we wanted smaller, easy-to-read PRs :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a clear separation of responsibilities :P

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean you're welcome to add documentation that describes how (3) will work, I guess, but its a bit confusing...whatever 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do!

TheBlueMatt
TheBlueMatt previously approved these changes May 31, 2022
View reviewed changes
@TheBlueMatt
Copy link
Collaborator

Can you go ahead and squash? We should try to land this and the BP changes tomorrow and cut 107, there's not much left that isn't trivial.

@arik-so
Copy link
Contributor Author

arik-so commented May 31, 2022

Yup! 🎃

@arik-so
Copy link
Contributor Author

arik-so commented May 31, 2022

It's technically a gourd, but the closest I could find to a squash emoji.

@arik-so arik-so force-pushed the 2022-05-network-graph-rapid-sync-timestamp branch from fd75747 to ffed82c Compare May 31, 2022 06:12
@arik-so arik-so requested a review from TheBlueMatt May 31, 2022 16:58
TheBlueMatt
TheBlueMatt previously approved these changes May 31, 2022
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still kinda hate that this PR leaves the repo in an intermediate state where we have docs that are incorrect based on a as-yet-unimplemented PR, but I'm willing to live with it here given that's just the one thing we have to land for 107 anyway.

@TheBlueMatt TheBlueMatt added this to the 0.0.107 milestone May 31, 2022
@arik-so
Copy link
Contributor Author

arik-so commented May 31, 2022

I hear you, but it's either that or massive PRs.

@TheBlueMatt
Copy link
Collaborator

In this case it really isn't, though - its one line on the gossip sync crate to update this field when we run an update.

valentinewallace
valentinewallace reviewed May 31, 2022
View reviewed changes
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we explain this change a little more in the commit message?

Other than that plus these nits, LGTM

@arik-so
Add optional last_rapid_gossip_sync_timestamp field to NetworkGraph t…
c3bbfe5 
…o enable optimized differental rapid syncing.
@arik-so arik-so force-pushed the 2022-05-network-graph-rapid-sync-timestamp branch from ffed82c to c3bbfe5 Compare May 31, 2022 17:17
@arik-so
Copy link
Contributor Author

arik-so commented May 31, 2022

Updated commit message; lmk if I should rename the PR, too. @valentinewallace

@arik-so arik-so merged commit 0b77008 into lightningdevkit:main May 31, 2022
@arik-so arik-so mentioned this pull request Feb 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@valentinewallace valentinewallace valentinewallace approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.0.107
Development

Successfully merging this pull request may close these issues.
4 participants
@arik-so @codecov-commenter @TheBlueMatt @valentinewallace